feat: `local-orch-account` `.transfer()` supports pfm routes #10571

0xpatrickdev · 2024-11-26T05:53:55Z

refs: #10445

Description

leverages ChainHub.makeTransferRoute to support PFM routing in LocalOrchAccount.transfer()
updates test harness to supply chain and asset info for chainHub in exo and contract tests
refactors send-anywhere contract-upgrade.test.ts to no longer rely on a buggy agoricNames
updates several orchestration examples contracts to seed their own chainHub to facilitate boot and multichain testing

Security Considerations

No new considerations from these changes.

Scaling Considerations

No new considerations from these changes.

Documentation Considerations

None

Testing Considerations

Includes unit tests. See #10584 for more robust testing of the pfm route logic. Existing multichain-testing will cover potential regressions (single-hop) transfers. Multi-hop tests are forthcoming.

Upgrade Considerations

N/A, library code. Part of NPM Orch or FUSDC release.

cloudflare-workers-and-pages · 2024-11-26T05:56:44Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`a2d491f`
Status:	✅ Deploy successful!
Preview URL:	https://c2569010.agoric-sdk.pages.dev
Branch Preview URL:	https://pc-10445-transfer-pfm.agoric-sdk.pages.dev

View logs

github-actions · 2024-11-26T17:07:00Z

Base branch is changed to master. Please re-run the integration tests by adding 'force:integration' label.

packages/orchestration/src/exos/local-orchestration-account.js

multichain-testing/test/support.ts

0xpatrickdev · 2024-11-30T01:05:24Z

This is now dependent on #10588.

As of writing I see green minus some seemingly unrelated inter integration tests.

dckc

looks pretty good

If any of my comments turn out to be important, I'm confident that we can address them in due course.

dckc · 2024-11-30T00:02:34Z

packages/orchestration/src/exos/local-orchestration-account.js

-              ),
+            const { forwardOpts, ...rest } = opts ?? {};
+
+            // throws if route is not determinable


hoist that to a @throws on the transfer method?

dckc · 2024-11-30T00:07:21Z

packages/orchestration/src/exos/local-orchestration-account.js

@@ -686,12 +682,16 @@ export const prepareLocalOrchestrationAccountKit = (
        transfer(destination, amount, opts) {
          return asVow(() => {
            trace('Transferring funds from LCA over IBC');
+            const denomAmount = coerceDenomAmount(chainHub, amount);


This can throw, right? Add a @throws in the transfer docstring in orchestration-api.ts?

I see TODO document the mapping from the address to the destination chain.

Is it time to document that now?

Updated here and orchestration-api.ts

dckc · 2024-11-30T00:09:43Z

packages/orchestration/test/exos/local-orchestration-account-kit.test.ts

-    { message: /connection not found: agoric-3<->fakenet/ },
+    { message: 'no connection info found for "agoric-3_fakenet"' },


I kinda like <->. I wonder why it went away.

dckc · 2024-11-30T00:18:40Z

packages/orchestration/src/utils/asset.js

- * @returns {[string, DenomDetail & { brandKey?: string }]}
+ * @returns {[string, DenomDetail]}


dckc · 2024-11-30T00:21:03Z

packages/orchestration/test/supports.ts

+    assetOn('ubld', 'agoric', bldSansMint.brand),
+    assetOn('uist', 'agoric', istSansMint.brand),
+    assetOn('uusdc', 'noble', undefined, 'agoric', chainInfoWithCaps),
+    assetOn('uatom', 'cosmoshub', undefined, 'agoric', chainInfoWithCaps),


no ATOM brand? that's a little odd.

We don't have one created in test support yet. I guess no tests currently call for it.

dckc · 2024-11-30T00:42:42Z

packages/orchestration/src/exos/cosmos-orchestration-account.js

+            console.warn(
+              '⚠️ `amountToCoin` not working until #10449; use `DenomAmount` for now',


hm. "death before confusion" suggests throwing here.

Updated to throw here. Had to refactor the staking-combinations example contract a bit as a result

dckc · 2024-11-30T00:48:08Z

packages/orchestration/src/examples/auto-stake-it.contract.js

@@ -67,6 +71,13 @@ const contract = async (

  const creatorFacet = prepareChainHubAdmin(zone, chainHub);

+  registerChainsAndAssets(


Thinking about the way each "feat" shows up in release notes, I wonder if "feat: auto-stake-it provides its own chain, connection, asset info" would make a better commit message.

Or, since auto-stake-it is more of an example than an orch API product feature, call it a chore.

Good feedback, updated

dckc · 2024-11-30T00:53:00Z

packages/boot/test/orchestration/restart-contracts.test.ts

+      '--assetInfo',
+      JSON.stringify([
+        [
+          'uist',
+          {
+            baseDenom: 'uist',
+            baseName: 'agoric',
+            chainName: 'agoric',
+          },


no IST brand. hm. I guess this test doesn't need it?

toExternalConfig would work nicely here instead of JSON.stringify. Just like...

agoric-sdk/packages/builders/scripts/fast-usdc/init-fast-usdc.js

Line 213 in d8a109e

options: toExternalConfig(config, crossVatContext, FastUSDCConfigShape),

toExternalConfig would work nicely here instead of JSON.stringify. Just like...

Agree, but I did not tackle this here. Something to improve in the future.

dckc · 2024-11-30T01:10:06Z

packages/orchestration/src/proposals/start-basic-flows.js

+      startUpgradable,
+    },
+    installation: {
+      // @ts-expect-error not a WellKnownName


elsewhere we refine the powers type to tell callers what's really expected

dckc · 2024-11-30T01:13:23Z

packages/fast-usdc/test/fast-usdc.contract.test.ts

@@ -652,17 +656,24 @@ const makeCustomer = (
        { amount: String(toReceive.value), denom: uusdcOnAgoric },
        'C4',
      );
-
-      t.log(who, 'sees', ibcTransferMsg.token, 'sent to', EUD);


any particular reason to get rid of this t.log()?

No, restored

github-actions · 2024-12-03T21:43:27Z

Base branch is changed to master. Please re-run the integration tests by adding 'force:integration' label.

0xpatrickdev · 2024-12-03T21:58:54Z

@mergify refresh

mergify · 2024-12-03T21:58:58Z

refresh

✅ Pull request refreshed

0xpatrickdev · 2024-12-03T22:46:55Z

@mergify queue

mergify · 2024-12-03T22:46:59Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 9b8cad2

- shape used for pfm ForwardInfo options

- provide `chainInfo` and `assetInfo` in commonPrivateArgs for contracts to use - register common assets used in testing - provide `populateChainHub` function for use in exo unit testing in favor of `registerAgoricBld`

this test can no longer rely on the IST brand being available, so we - added checks to ensure brands are accepted for `.transfer` and `.send`. marked as failing for now - filed #10449 since this surfaced a bug in `amountToCoin` - use Moolah issuer for "no denom for brand" failure path tests

- enables sending offers with brands

- chainInfo and assetInfo are provided as `commonPrivateArgs`. include them in `ContractMeta`

- auto-stake-it initializes `chainHub` with data so existing tests that use `.transfer()` pass

- no longer needed since we call `registerChainsAndAssets`

- no longer relies on buggy agoricNames - continues to test async-flow resumability and cross-chain vow settlement across upgrade Co-authored-by: 0xPatrick <[email protected]>

- test/bootstrapTests/orchestration.test.ts used a mixed of test and test.serial - the test without `.serial` interleaved and was confusing to debug - this mainly updates inline snapshots, which return different account and channel identifiers since all tests in this file share the same context

- basic-flows.contract.js is provided with `chainInfo` and `assetInfo` in `privateArgs` via builder options - needed for tests that use `localAccount.transfer()`, now reliant on asset info, to pass

closes: #9966 closes: #10445 ## Description Adds multi-hop (PFM) scenarios to the `examples/send-anywhere.contract.js` multichain (e2e) test. To support this change, this PR also includes: - a proposal for registering interchain assets in vbank (closes #9966). aims for production quality but is only used in tests - a `fundFaucet` helper in `multichain-testing` so developers can request ATOM, OSMO, etc in `provisionSmartWallet` - a `GoDuration` type in `@agoric/orchestration` that captures basic Go [time duration strings](https://pkg.go.dev/time#ParseDuration) and an update to `DefaultPfmTimeoutOpts` (10min -> 10m) ### Security Considerations `@agoric/builders/scripts/testing/register-interchain-bank-assets.js` allows callers overwrite assets in `vbank` and `agoricNames`. It's only intended for testing, and shouldn't be used in production. A production version might guard against accidental overrides. ### Scaling Considerations None, mostly test code. Adds a little CI time to `multichain-testing` for the extra CoreEval. ### Documentation Considerations None ### Testing Considerations Includes an E2E to test in `multichain-testing` that leverages `register-interchain-bank-assets.js`. Also includes the first E2E test of PFM functionality added in #10584 and #10571. ### Upgrade Considerations None, library code an NPM orch or FUSDC release.

0xpatrickdev force-pushed the 10006-transfer-pfm branch from d6db026 to 2fa2f75 Compare November 26, 2024 07:07

0xpatrickdev force-pushed the pc/10445-transfer-pfm branch from c111b13 to 05dd05c Compare November 26, 2024 07:08

Base automatically changed from 10006-transfer-pfm to master November 26, 2024 17:06

0xpatrickdev commented Nov 26, 2024

View reviewed changes

packages/orchestration/src/exos/local-orchestration-account.js Outdated Show resolved Hide resolved

0xpatrickdev commented Nov 26, 2024

View reviewed changes

packages/orchestration/src/exos/local-orchestration-account.js Outdated Show resolved Hide resolved

0xpatrickdev force-pushed the pc/10445-transfer-pfm branch from 05dd05c to 310a176 Compare November 26, 2024 22:25

0xpatrickdev mentioned this pull request Nov 27, 2024

orchestration / fast-usdc: remove brandKey from assetInfo in privateArgs #10580

Open

0xpatrickdev force-pushed the pc/10445-transfer-pfm branch 2 times, most recently from 5977597 to 9c0a243 Compare November 28, 2024 03:02

0xpatrickdev added the force:integration Force integration tests to run on PR label Nov 28, 2024

0xpatrickdev force-pushed the pc/10445-transfer-pfm branch from af67a1f to e6de422 Compare November 28, 2024 04:32

0xpatrickdev commented Nov 28, 2024

View reviewed changes

multichain-testing/test/support.ts Outdated Show resolved Hide resolved

0xpatrickdev marked this pull request as ready for review November 28, 2024 04:44

0xpatrickdev requested a review from a team as a code owner November 28, 2024 04:44

0xpatrickdev requested review from AgoricTriage, turadg, dckc and samsiegart November 28, 2024 04:44

0xpatrickdev force-pushed the pc/10445-transfer-pfm branch from e6de422 to 81d1df5 Compare November 28, 2024 06:32

0xpatrickdev marked this pull request as draft November 29, 2024 04:31

0xpatrickdev mentioned this pull request Nov 29, 2024

feat!: chainHub.getAsset requires srcChainName parameter #10588

Merged

0xpatrickdev force-pushed the pc/10445-transfer-pfm branch from 81d1df5 to 6c5db7f Compare November 29, 2024 23:44

0xpatrickdev changed the base branch from master to pc/10445-chain-hub-get-asset November 29, 2024 23:45

0xpatrickdev marked this pull request as ready for review November 30, 2024 01:04

dckc approved these changes Nov 30, 2024

View reviewed changes

0xpatrickdev mentioned this pull request Dec 1, 2024

test: send-anywhere pfm scenarios #10591

Merged

0xpatrickdev force-pushed the pc/10445-chain-hub-get-asset branch from c12d868 to aa03864 Compare December 2, 2024 16:03

0xpatrickdev removed the force:integration Force integration tests to run on PR label Dec 3, 2024

0xpatrickdev force-pushed the pc/10445-transfer-pfm branch from a1132f4 to a706d28 Compare December 3, 2024 22:34

0xpatrickdev and others added 19 commits December 3, 2024 23:42

feat: ForwardOptsShape

50b1717

- shape used for pfm ForwardInfo options

feat(local-orchestration-account): support multi-hop pfm transfers

c35fac7

test: chainHub test support

05aa139

- provide `chainInfo` and `assetInfo` in commonPrivateArgs for contracts to use - register common assets used in testing - provide `populateChainHub` function for use in exo unit testing in favor of `registerAgoricBld`

timestamp durable

e8b37a8

chore: send-anywhere includes IST and BLD brands

20cf6d4

- enables sending offers with brands

chore: update send-anywhere baggage snapshot

09a3a92

chore: auto-stake-it provides its own chain, connection, asset info

b39a332

chore: swap.contract.js allows chainInfo and assetInfo

cf51484

- chainInfo and assetInfo are provided as `commonPrivateArgs`. include them in `ContractMeta`

test(boot): restart send-anywhere includes asset info

68c568a

chore: auto-stake-it.contract.js inits chainHub

56164bd

- auto-stake-it initializes `chainHub` with data so existing tests that use `.transfer()` pass

chore(fusdc): remove write-chain-info.js

2195ace

- no longer needed since we call `registerChainsAndAssets`

test: send-anywhere contract null upgrade

f5b8b22

- no longer relies on buggy agoricNames - continues to test async-flow resumability and cross-chain vow settlement across upgrade Co-authored-by: 0xPatrick <[email protected]>

test(send-anywhere): explicitly check result key is undefined

a93b624

chore: basic-flows.contract.js seeds chainHub

df1af7a

- basic-flows.contract.js is provided with `chainInfo` and `assetInfo` in `privateArgs` via builder options - needed for tests that use `localAccount.transfer()`, now reliant on asset info, to pass

test(fast-usdc): support pfm transfers

fa9b36d

test: deposit-withdraw requires asset info

fe39f0a

chore: startContract stringifies builderOpts

a2d491f

0xpatrickdev force-pushed the pc/10445-transfer-pfm branch from a706d28 to a2d491f Compare December 3, 2024 23:43

mergify bot merged commit 9b8cad2 into master Dec 4, 2024
81 checks passed

mergify bot deleted the pc/10445-transfer-pfm branch December 4, 2024 00:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: `local-orch-account` `.transfer()` supports pfm routes #10571

feat: `local-orch-account` `.transfer()` supports pfm routes #10571

0xpatrickdev commented Nov 26, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Nov 26, 2024 •

edited

Loading

github-actions bot commented Nov 26, 2024

0xpatrickdev commented Nov 30, 2024

dckc left a comment

dckc Nov 30, 2024

dckc Nov 30, 2024

0xpatrickdev Dec 3, 2024

dckc Nov 30, 2024

0xpatrickdev Dec 3, 2024

dckc Nov 30, 2024

dckc Nov 30, 2024

0xpatrickdev Dec 3, 2024

dckc Nov 30, 2024

0xpatrickdev Dec 3, 2024

dckc Nov 30, 2024

0xpatrickdev Dec 3, 2024

dckc Nov 30, 2024

0xpatrickdev Dec 3, 2024

dckc Nov 30, 2024

dckc Nov 30, 2024

0xpatrickdev Dec 3, 2024

github-actions bot commented Dec 3, 2024

0xpatrickdev commented Dec 3, 2024

mergify bot commented Dec 3, 2024

0xpatrickdev commented Dec 3, 2024

mergify bot commented Dec 3, 2024 •

edited

Loading

		{ message: /connection not found: agoric-3<->fakenet/ },
		{ message: 'no connection info found for "agoric-3_fakenet"' },

		* @returns {[string, DenomDetail & { brandKey?: string }]}
		* @returns {[string, DenomDetail]}

		console.warn(
		'⚠️ `amountToCoin` not working until #10449; use `DenomAmount` for now',

		@@ -67,6 +71,13 @@ const contract = async (

		const creatorFacet = prepareChainHubAdmin(zone, chainHub);

		registerChainsAndAssets(

feat: local-orch-account .transfer() supports pfm routes #10571

feat: local-orch-account .transfer() supports pfm routes #10571

Conversation

0xpatrickdev commented Nov 26, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Nov 26, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

github-actions bot commented Nov 26, 2024

0xpatrickdev commented Nov 30, 2024

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 3, 2024

0xpatrickdev commented Dec 3, 2024

mergify bot commented Dec 3, 2024

✅ Pull request refreshed

0xpatrickdev commented Dec 3, 2024

mergify bot commented Dec 3, 2024 • edited Loading

✅ The pull request has been merged automatically

feat: `local-orch-account` `.transfer()` supports pfm routes #10571

feat: `local-orch-account` `.transfer()` supports pfm routes #10571

0xpatrickdev commented Nov 26, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Nov 26, 2024 •

edited

Loading

mergify bot commented Dec 3, 2024 •

edited

Loading